feat: add protocol version validation to SafeCodec #158
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR adds backward-compatible protocol version validation to SafeCodec. A Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f2bd079fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
bbopen
changed the base branch from
feat/adr-002-bridge-protocol-migration
to
main
Implements #154 - Protocol version validation Changes: - Add validateProtocolVersion() function to safe-codec.ts - Validate protocol field in decodeResponse and decodeResponseAsync - Accept missing protocol field for backwards compatibility - Reject mismatched protocol versions with clear error messages - Add 5 unit tests for protocol validation - Enable wrong_protocol_bridge.py adversarial test The validation uses PROTOCOL_ID constant ('tywrap/1') from transport.ts and throws BridgeProtocolError on version mismatch. Fixes #154 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
bbopen
force-pushed
the
feat/protocol-version-validation
branch
from
9f2bd07 to
3fbde99
Compare
coderabbitai
bot
added
area:codec
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/safe-codec.test.ts`:
- Around line 689-730: Add a test to ensure SafeCodec.decodeResponse allows user
result objects that themselves contain a "protocol" field: in the existing
"decodeResponse - Protocol Validation" suite add an it() that builds payload =
JSON.stringify({ id: 1, protocol: 'tywrap/1', result: { protocol: 'http', url:
'example.com' } }), calls codec.decodeResponse<{ protocol: string; url: string
}>(payload) and expects the returned value toEqual { protocol: 'http', url:
'example.com' } (and not throw); this verifies decodeResponse handles envelope
protocol validation without rejecting user data that has a protocol property.
♻️ Duplicate comments (1)
src/runtime/safe-codec.ts (1)
165-180: Restrict protocol validation to protocol envelopes only.This validation runs on any object with a
protocolfield, which is a behavioral regression. A legitimate user result like{ "protocol": "http", "url": "..." }returned directly (not wrapped in an envelope) will incorrectly throwBridgeProtocolError.The validation should only apply when the payload is a protocol envelope (i.e., has
idwithresultorerror).🐛 Proposed fix
function validateProtocolVersion(value: unknown): void { if (value === null || typeof value !== 'object') { return; } const obj = value as Record<string, unknown>; + // Only validate protocol on actual protocol envelopes (must have 'id' field) + if (!('id' in obj && typeof obj.id === 'number')) { + return; + } if ('protocol' in obj && obj.protocol !== PROTOCOL_ID) { throw new BridgeProtocolError( `Invalid protocol version: expected "${PROTOCOL_ID}", got "${obj.protocol}"` ); } }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/runtime/safe-codec.tstest/adversarial_playground.test.tstest/safe-codec.test.ts
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:168-172
Timestamp: 2026-01-20T16:00:49.738Z
Learning: In the tywrap project's BridgeProtocol SafeCodec implementation, Arrow format decoders can produce NaN/Infinity values from binary representations even when the raw JSON payload doesn't contain them. This is why validation for special floats must occur both before encoding (to reject invalid inputs) and after applying decoders (to catch values introduced during Arrow deserialization), protecting downstream consumers from unexpected NaN/Infinity values.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:203-211
Timestamp: 2026-01-20T16:00:58.747Z
Learning: In the BridgeProtocol implementation for the tywrap repository, Map and Set instances should be explicitly rejected before serialization (e.g., in `safeStringify` or at the serialization entrypoint) with a clear error message like "Bridge protocol does not support Map/Set values", rather than checking for non-string keys after the fact, since Maps and Sets cannot round-trip through JSON.stringify.
📚 Learning: 2026-01-19T21:00:30.825Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:30.825Z
Learning: In adversarial tests for the bridge (test/adversarial_playground.test.ts), recovery assertions should be kept in the finally block adjacent to cleanup to verify resilience under stress, even if that means post-timeout checks might mask the original error. The goal is to surface recovery failures as legitimate test failures.
Applied to files:
test/adversarial_playground.test.tstest/safe-codec.test.ts
📚 Learning: 2026-01-19T21:48:27.823Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/runtime_bridge_fixtures.test.ts:59-66
Timestamp: 2026-01-19T21:48:27.823Z
Learning: In fixture-based tests (e.g., test/runtime_bridge_fixtures.test.ts) and similar tests in the tywrap repository, prefer early returns when Python or fixture files are unavailable. Do not rely on Vitest dynamic skip APIs; a silent pass is intentional for environments lacking Python/fixtures. Treat missing fixtures as optional soft-guards and ensure the test remains non-disruptive in non-availability scenarios.
Applied to files:
test/adversarial_playground.test.tstest/safe-codec.test.ts
📚 Learning: 2026-01-19T21:14:40.872Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Applied to files:
test/adversarial_playground.test.tstest/safe-codec.test.ts
📚 Learning: 2026-01-19T21:48:45.693Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Applied to files:
test/adversarial_playground.test.tssrc/runtime/safe-codec.tstest/safe-codec.test.ts
📚 Learning: 2026-01-19T21:00:52.689Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/fixtures/out_of_order_bridge.py:29-48
Timestamp: 2026-01-19T21:00:52.689Z
Learning: In `test/fixtures/out_of_order_bridge.py`, the fixture intentionally leaves a pending request unanswered at EOF to simulate missing/out-of-order responses and validate bridge behavior when requests never complete; this is the exact failure mode being tested and must be preserved.
Applied to files:
test/adversarial_playground.test.ts
📚 Learning: 2026-01-19T21:00:52.689Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/fixtures/out_of_order_bridge.py:29-48
Timestamp: 2026-01-19T21:00:52.689Z
Learning: Policy for adversarial test fixtures: adversarial fixtures must preserve the failure mode they exercise, even if that behavior would be "bad" in real code. When reviewing test fixtures with unusual patterns (e.g., missing error handling, intentional protocol violations, unanswered requests), ask for clarifying comments about the intent rather than suggesting normalization or fixes.
Applied to files:
test/adversarial_playground.test.ts
📚 Learning: 2026-01-20T16:00:49.829Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:203-211
Timestamp: 2026-01-20T16:00:49.829Z
Learning: In the BridgeProtocol implementation (tywrap), reject Map and Set explicitly before serialization (e.g., in safeStringify or the serialization entrypoint) with a clear error like "Bridge protocol does not support Map/Set values". Do not rely on post-hoc checks of non-string keys at the point of JSON.stringify, since Maps/Sets cannot round-trip. This should be enforced at the boundary where data is prepared for serialization to ensure deterministic errors and prevent invalid data from propagating.
Applied to files:
test/adversarial_playground.test.tssrc/runtime/safe-codec.tstest/safe-codec.test.ts
📚 Learning: 2026-01-20T01:34:07.064Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:444-458
Timestamp: 2026-01-20T01:34:07.064Z
Learning: When reviewing promise-based polling patterns (e.g., recursive setTimeout in waitForAvailableWorker functions), ensure that any recursive timer is tracked and cleared on timeout or promise resolution to prevent timer leaks. Do not rely on no-op checks after settlement; verify clearTimeout is invoked in all paths and consider using an explicit cancellation (e.g., AbortController) or a guard to cancel pending timers when the promise settles.
Applied to files:
test/adversarial_playground.test.tssrc/runtime/safe-codec.tstest/safe-codec.test.ts
📚 Learning: 2026-01-20T18:37:05.670Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: src/runtime/process-io.ts:37-40
Timestamp: 2026-01-20T18:37:05.670Z
Learning: In the tywrap repo, ESLint is used for linting (not Biome). Do not flag regex literals that contain control characters (e.g., \u001b, \u0000-\u001F) as lint errors, since the current ESLint configuration allows them. When reviewing TypeScript files (e.g., src/**/*.ts), rely on ESLint results and avoid raising issues about these specific control-character regex literals unless there is a competing config change or a policy requiring explicit escaping.
Applied to files:
test/adversarial_playground.test.tssrc/runtime/safe-codec.tstest/safe-codec.test.ts
📚 Learning: 2026-01-19T21:14:35.390Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:35.390Z
Learning: In src/runtime/bridge-core.ts and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (such as runtime, shape, or error-payload checks) inside tight loops unless the protocol boundary is untrusted or there is a concrete bug report. The Python bridge protocol is controlled via tests, so these checks add unnecessary branching overhead without meaningful benefit. Apply this guidance to other hot-path runtime loops in src/runtime/**/*.ts, and re-enable additional validations only when a documented risk or failure scenario is identified. Ensure tests cover protocol validation where applicable.
Applied to files:
src/runtime/safe-codec.ts
📚 Learning: 2026-01-20T16:00:49.738Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:168-172
Timestamp: 2026-01-20T16:00:49.738Z
Learning: In the tywrap project's BridgeProtocol SafeCodec implementation, Arrow format decoders can produce NaN/Infinity values from binary representations even when the raw JSON payload doesn't contain them. This is why validation for special floats must occur both before encoding (to reject invalid inputs) and after applying decoders (to catch values introduced during Arrow deserialization), protecting downstream consumers from unexpected NaN/Infinity values.
Applied to files:
src/runtime/safe-codec.tstest/safe-codec.test.ts
📚 Learning: 2026-01-19T21:14:29.869Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:29.869Z
Learning: In the runtime env-var parsing (e.g., in src/runtime/bridge-core.ts and similar modules), adopt a tolerant, best-effort policy: numeric env values should parse by taking the leading numeric portion (e.g., TYWRAP_CODEC_MAX_BYTES=1024abc -> 1024). Only reject clearly invalid values (non-numeric start or <= 0). This reduces surprising failures from minor typos. Add tests to cover partial numeric prefixes, and ensure downstream logic documents the trusted range; consider a small helper to extract a positive integer from a string with a safe fallback.
Applied to files:
src/runtime/safe-codec.ts
🧬 Code graph analysis (1)
src/runtime/safe-codec.ts (1)
src/runtime/transport.ts (1)
PROTOCOL_ID(20-20)
🔇 Additional comments (5)
test/adversarial_playground.test.ts (1)
488-491: LGTM!The test is correctly enabled now that protocol version validation is implemented in SafeCodec. The expected pattern
/Invalid protocol/aligns with the error message fromvalidateProtocolVersion().src/runtime/safe-codec.ts (4)
14-14: LGTM!Import of
PROTOCOL_IDfrom transport is appropriate for centralizing the protocol version constant.
335-336: Protocol validation placement is correct.Validating protocol version before error extraction aligns with the PR objective that protocol mismatch should be detected before attempting to process the payload content.
388-389: Consistent with sync version.Protocol validation in the async path mirrors the sync
decodeResponsemethod, maintaining consistency.
151-163: LGTM!The
ProtocolResultResponseinterface correctly makesprotocoloptional for backwards compatibility, and the type guard properly identifies protocol envelopes by checking foridandresultfields.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Address review feedback: only validate protocol field when the response looks like a protocol envelope (has 'id' field). This prevents false positives on user data that happens to contain a 'protocol' key. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Implements #154 - Protocol version validation in the SafeCodec layer.
validateProtocolVersion()function that checks theprotocolfield in responsesPROTOCOL_IDconstant (tywrap/1) from transport.tswrong_protocol_bridge.pyadversarial testChanges
src/runtime/safe-codec.tsvalidateProtocolVersion()function and calls in decode methodstest/safe-codec.test.tstest/adversarial_playground.test.tswrong_protocol_bridge.pytestTest plan
wrong_protocol_bridge.pyadversarial test enabled and passingNote: This PR is based on
feat/adr-002-bridge-protocol-migration(PR #153) since it depends on SafeCodec. Should be merged after #153.Fixes #154
🤖 Generated with Claude Code